Route owned PR events to babysitter sessions#92
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesPR babysitter wake flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant FactoryLoop
participant BabysitterSession
GitHub->>FactoryLoop: owned PR review or check event
FactoryLoop->>FactoryLoop: coalesce wake kinds and check critical fence
FactoryLoop->>BabysitterSession: inject metadata-only wake
BabysitterSession-->>FactoryLoop: critical begin/end signal
FactoryLoop->>BabysitterSession: submit deferred wake
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
61fa3e3 to
e41931e
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/orchestrator/factory.ts (1)
7676-7679: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject PR numbers that disagree with the canonical path.
payload.numbersilently overridesfallbackNumber. Thus content for/pulls/420/...can claim PR 421, producing ownership for 421 while persisting the path for 420.Proposed fix
const parsePullSnapshot = (content: unknown, fallbackNumber: number): PullSnapshot | undefined => { const payload = wrappedPayload(content) - const number = typeof payload.number === 'number' ? payload.number : fallbackNumber + const explicitNumber = payload.number === undefined + ? undefined + : positiveIntegerLike(payload.number) + if (payload.number !== undefined && explicitNumber !== fallbackNumber) return undefined + const number = explicitNumber ?? fallbackNumber if (!Number.isInteger(number) || number <= 0) return undefined🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrator/factory.ts` around lines 7676 - 7679, Update parsePullSnapshot so fallbackNumber from the canonical path is authoritative, rather than allowing payload.number to override it. Validate the payload number when present and return undefined if it is not an integer or disagrees with fallbackNumber; otherwise continue using fallbackNumber for the resulting PullSnapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/orchestrator/factory.ts`:
- Around line 3149-3152: In the babysitter critical-section exit flow around
`#finishBabysitterCriticalSection` and deferred wake processing, persist the
cleared critical fence immediately after consuming the end signal and before
submitting any deferred PTY input or wakes. Preserve the existing deferred
submission behavior, but ensure `#persistBabysitterCriticalFence` runs before that
processing so a crash cannot restore a permanent fence.
- Around line 4137-4149: The delivery-failure handling near the wake-delivery
path must check whether the wake was cancelled before restoring kinds or
persisting its session. Update the logic around `#cancelBabysitterWake` and the
failure branch to skip session recreation and related recovery for cancelled
state, preserving cleanup for active wakes.
- Around line 4495-4498: The readiness handling around the babysitter-enabled
guard must reject stale signals after PR ownership is cancelled. Before
resolving the agent, require that the babysitter still has active ownership and
that `#readBabysatPrSnapshot`() returns an authoritative snapshot; otherwise
cancel or terminate the babysitter and return. Preserve the existing non-OPEN
snapshot cancellation behavior.
- Around line 4505-4517: Update the ownership establishment flow around
`#inFlightIssueForPrSnapshot` and the babysitter reservation at the referenced
ownership block so a first match cannot claim ownership from title/body
references alone. Require either authoritative published-PR identity or a strong
head-branch association before creating the initial babysitter ownership; reject
weak same-repository references such as only “Fixes AR-420” when no existing
owner is present, while preserving established-owner mismatch handling.
- Around line 4331-4343: Update the babysitter wake transition around
`#recordPendingBabysitterWake` and `#flushBabysitterWake` so failures are handled by
recovery logic covering the entire flush sequence. On rejection, restore
deliveringKinds into kinds, schedule a retry through `#scheduleBabysitterWake`,
and ensure the promise chain ends with a catch so the ignored finally promise
cannot reject unhandled. Apply the same recovery to the corresponding wake path
noted at the additional location.
- Around line 3118-3120: Update the issue-key parser/normalizer feeding
durableIssue and babysitterCritical to accept GitHub keys in
owner/repository#number format, such as AgentWorkforce/hoopsheet#10, alongside
existing AR and numeric keys. Preserve normalization and ensure the resulting
key is recognized by both destructive-operation fence checks, including the
corresponding logic near the alternate location.
In `@src/state/file-state-store.ts`:
- Line 566: Update the persisted-record validation around candidate.critical so
missing or undefined values are rejected, requiring critical to be a boolean
rather than accepting undefined and restoring false. Apply the same strict
validation to the corresponding check at the other referenced location, while
preserving existing handling for valid boolean values.
In `@src/triage/schema.ts`:
- Around line 18-27: Remove ownedPullRequest and pendingPullRequestWake from
TriageDecisionSchema, keeping them available only on the internal AgentSpec
shape. Update LlmTriage or the decision-to-agent-state storage path to strip or
otherwise prevent these fields from parsed model output, while preserving
babysitter lifecycle code as the only source that sets them.
---
Outside diff comments:
In `@src/orchestrator/factory.ts`:
- Around line 7676-7679: Update parsePullSnapshot so fallbackNumber from the
canonical path is authoritative, rather than allowing payload.number to override
it. Validate the payload number when present and return undefined if it is not
an integer or disagrees with fallbackNumber; otherwise continue using
fallbackNumber for the resulting PullSnapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 726c270d-7a63-421f-9dde-2fbe7b8f45fc
📒 Files selected for processing (10)
src/dispatch/templates.test.tssrc/dispatch/templates.tssrc/orchestrator/factory.test.tssrc/orchestrator/factory.tssrc/ports/fleet.tssrc/ports/state.tssrc/state/file-state-store.test.tssrc/state/file-state-store.tssrc/state/in-memory-state-store.tssrc/triage/schema.ts
|
Addressed the outside-diff PR-number finding in 7bb575d: the canonical path number is authoritative; an explicit payload number must parse as a positive integer and equal the path, otherwise the snapshot is rejected. Added a path/payload mismatch regression. All nine CodeRabbit findings were independently audited and signed off on exact freeze 7bb575d. |
Summary
Stack
Depends on #89. This PR is intentionally stacked on branch codex/issue-82-repo-agent-names and preserves its repo-qualified agent naming plus composite issue identity. It must not target main until #89 merges; after that, it requires rebase/integration and a fresh frozen shadow delta audit before changing the base.
Fixes #87
Security and lifecycle
Validation